Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: implement jj root #2801

Merged
merged 1 commit into from
Jan 25, 2024
Merged

cli: implement jj root #2801

merged 1 commit into from
Jan 25, 2024

Conversation

v-gb
Copy link
Collaborator

@v-gb v-gb commented Jan 10, 2024

This is a convenient command when scripting around, for things like: $ cd $(jj root) && do something

Currently this fails if the path to the repo contains non-utf8 bytes, similarly to jj status failing on non-utf8 paths inside the repo.

This is a proposal out of the blue, but since this is a small change and it seems not contentious, I figured if it was simpler to do it and possibly argue about the utility of the PR, rather than have a round of discussion followed by a round of PR.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

@joyously
Copy link

The test handles only one case?

@v-gb v-gb force-pushed the push-ozxsyrvuklqw branch from 4d7a040 to ccbea9e Compare January 10, 2024 19:02
@v-gb
Copy link
Collaborator Author

v-gb commented Jan 10, 2024

I forgot to put a test for the normal failure case, it's added now.

Did you have specific tests in mind? I could cover the non-utf8 error, but it's not obviously worth it. If jj supports worktrees/shares, I guess that could be tested as well, it depends to what extent we trust on the internal apis forcing callers to do the right things.

@joyously
Copy link

I was thinking that there are more options to init and Git might not be involved.
#2747

@PhilipMetzger
Copy link
Collaborator

This is a convenient command when scripting around, for things like: $ cd $(jj root) && do something

Currently this fails if the path to the repo contains non-utf8 bytes, similarly to jj status failing on non-utf8 paths inside the repo.

I like this, as it obviously useful for scripting. On the whole UTF-8 path topic, @martinvonz mentioned quite a while ago that jj should move to Camino for paths anyway.

@v-gb v-gb force-pushed the push-ozxsyrvuklqw branch from ccbea9e to e887ffb Compare January 10, 2024 21:08
@v-gb
Copy link
Collaborator Author

v-gb commented Jan 10, 2024

I was thinking that there are more options to init and Git might not be involved.

I made the test run with both jj init and jj init --git.
(I actually got confused by the bit of test code that replaces the tmp dir in jj's output by $TEST_ENV. It didn't work on the previous version of my test, because the rewriting only happens when the tmp dir is followed by something, like a slash. It's hard to dig into the code to confirm there's nothing sneaky going on without jj blame, sadly!).

Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some nits, I think it's also missing a test for multiple workspaces.

cli/src/commands/root.rs Show resolved Hide resolved
cli/tests/test_root.rs Outdated Show resolved Hide resolved
@v-gb
Copy link
Collaborator Author

v-gb commented Jan 10, 2024

I was looking at making a test involving workspaces, and AFAICT, jj workspace root is already exactly jj root?

Should I move that command to the toplevel then? It does seem that the need for the repo root should come before the need for workspaces. If that's the plan, should I preserve some compatibility of the CLI?

@martinvonz
Copy link
Member

I was looking at making a test involving workspaces, and AFAICT, jj workspace root is already exactly jj root?

Yes, it does sound like it's the same.

Should I move that command to the toplevel then? It does seem that the need for the repo root should come before the need for workspaces. If that's the plan, should I preserve some compatibility of the CLI?

The reason it's under jj workspace is that the command returns the workspace's root, to clarify that that if you're in a different workspace, you'll get a different root directory.

Maybe we could add jj root as an alias for jj workspace root if we think it's a common enough command, or - maybe more likely - if we think it's hard to find under jj workspace.

@v-gb v-gb force-pushed the push-ozxsyrvuklqw branch from e887ffb to a3fefcf Compare January 10, 2024 23:48
@v-gb
Copy link
Collaborator Author

v-gb commented Jan 11, 2024

The reason it's under jj workspace is that the command returns the workspace's root, to clarify that that if you're in a different workspace, you'll get a different root directory.

Maybe we could add jj root as an alias for jj workspace root if we think it's a common enough command, or - maybe more likely - if we think it's hard to find under jj workspace.

The reasoning makes sense. As you say, for me it's the discoverability that makes me prefer having the command at the top of the command line (well, that and the fact that status, commit, @, config all act on the current workspace without needing to say so, and the fact that that I haven't seen people get confused by hg root + shares, or git rev-parse --show-toplevel + worktrees).

I tweaked the PR to:

  • replace my jj root with the one from jj workspace root (same behavior, but cleaner)
  • keep jj workspace root as a hidden alias to jj root, to avoid breaking stuff but still favor jj root

Finally, the existing tests of jj workspace root cover tests that use workspaces, so that should be everything !

@martinvonz
Copy link
Member

martinvonz commented Jan 11, 2024

jj workspace root was a little clearer about what it prints the root for; I find jj root is less clear. Do you think we should call it jj repo-root or jj repo-path? jj workspace-root would be more technically correct since it prints the workspace's root, not the repo's root (which could be considered to be .jj/repo/), but that's similar enough to the current jj workspace root that I suspect you might not like it. Or maybe you would still find it almost as easily as jj root if it's a top-level command?

@ilyagr
Copy link
Collaborator

ilyagr commented Jan 11, 2024

UPDATE: I originally added this thought at the end, but I now decided I like it: we could have jj root --git, jj root --repo, and default to jj root --workspace. It could also be an alias for the commands I mentioned before (see the rest of this comment).


I think jj repo-root should point to the .jj/repo dir, convenient for finding the repo config. I would even consider making it a sub-command: jj repo root. It's an interesting question whether any other commands would go under jj repo.

I think a jj git root command would also be nice, for non-colocated repos.

@v-gb
Copy link
Collaborator Author

v-gb commented Jan 11, 2024

Well, this is more contentious of a change than I imagined it would be !

Given that I now know of the existence of jj workspace root, I added it to the git-comparison doc in another PR, as one way to improve discoverability. If we make more changes here, I'll just adjust the doc.

Another option would be to provide jj root, but make it fail (then it shows up in the help, and you can decide to turn it into a real command or remove at any point without backwards compatibility considerations).

jj workspace root was a little clearer about what it prints the root for; I find jj root is less clear. Do you think we should call it jj repo-root or jj repo-path? jj workspace-root would be more technically correct since it prints the workspace's root, not the repo's root (which could be considered to be .jj/repo/), but that's similar enough to the current jj workspace root, that I suspect you might not like it. Or maybe you would still find it almost as easily as jj root if it's a top-level command?

The terminology includes: repo, working copy and workspace. IIUC "working copy" means check-out + related state, "workspace" is an alias for "working copy" except with emphasis on the shared history ("two working copies" might mean two unrelated repos, but "two workspaces" probably means two working copies attached to the same repo), and "repo" is either the store directory, or the store + working copy whose .jj directory contains the store, or perhaps the store + all working copies.
So I would understand jj repo-root or jj repo-path as specifically referring to the store (or its working copy) and not the current working copy. jj workspace-root seems sufficiently awkward that I'd prefer to punt and see if we have a better idea in the future :).

We could have jj root --git, jj root --repo, and default to jj root --workspace

That sounds reasonable.
Although for rare options (how often do people need jj root --git?), a generic interface similar to jj config {list,get} could be preferable (it gives the user a one-stop shop to find rarer things that they can grep through, which you can't do with CLI commands and flag names due to clap not implementing recursive help, but even with recursive help, you couldn't grep for the value to find the name of the setting. And it's probably less effort to expose information this way, past the initial setup).

In any case, the options so far are:

  • change nothing (after advertising jj workspace root slightly in the git-comparison in the other PR. I would be ok with this, given that I now know the functionality exists)
  • add jj foo without other flags, for some value of foo that everyone is happy with, that we have yet to find
  • add jj root, but it fails and points to jj workspace root
  • jj root {--workspace,--git,--repo}, and it defaults to --workspace

@joyously
Copy link

Another option is to stick to verbs for commands, and use show root.

@PhilipMetzger
Copy link
Collaborator

we could have jj root --git, jj root --repo, and default to jj root --workspace.

I really like that idea, because as at some point jj run --bisect will have the same symmetry with jj bisect --run. This would also allow us to add a jj root --git for non-colocated repos, instead of fudging it into jj config get $git which was my previous suggestion.

Although for rare options (how often do people need jj root --git?), a generic interface similar to jj config {list,get} could be preferable

I'd rather not put the git repo in there, as it isn't configurable and just straight up confusing. This also was my previous idea when we talked about something like it.

@ilyagr
Copy link
Collaborator

ilyagr commented Jan 11, 2024

Well, this is more contentious of a change than I imagined it would be !

Sorry, these kind of non-urgent changes related to naming often lead to bike-sheddy discussions. I, personally, also like to ponder on the ideal outcome, even if it shouldn't necessarily be part of this particular PR.

In any case, the options so far are:

* change nothing (after advertising `jj workspace root` slightly in the git-comparison in the other PR. I would be ok with this, given that I now know the functionality exists)

* add `jj foo` without other flags, for some value of `foo` that everyone is happy with, that we have yet to find

* add `jj root`, but it fails and points to `jj workspace root`

* `jj root {--workspace,--git,--repo}`, and it defaults to --workspace

I, personally, would be happy with 1, 3, or 4. 2 is also fine if the prerequisite is satisfied :). If we go with 3, I'd ponder switching to 4 (or something else) later, but it could be a good solution until somebody has the energy to work on that.

@joyously

Another option is to stick to verbs for commands, and use show root.

This sounds very reasonable if we were designing the UI from scratch. Unfortunately, jj show is already a useful command that does something entirely different (prints a commit verbosely). In theory, we could consider changing that command as well. I can think of some arguments for that, but not a perfect solution, and I'm not sure it's worth it. It would have to be a whole separate discussion.

@martinvonz
Copy link
Member

The terminology includes: repo, working copy and workspace.

They are defined in the glossary.

jj root {--workspace,--git,--repo}, and it defaults to --workspace

I like this proposal, even if we decide to not add any of the flags yet. I don't really like the --git flag, though, since that adds Git-knowledge to another command at the same time as we're trying to remove it from jj init (#2747). It's probably not very

I still like having jj workspace root as the canonical form of the command because it's one of few commands that require a workspace to work. OTOH, we already have jj untrack and jj sparse outside of the jj workspace namespace. We need to be pragmatic, and maybe this is a case where I'm being too rigid; maybe we should just rename it to jj root as you originally proposed.

@martinvonz
Copy link
Member

So what do we do with this PR? I'm leaning towards keeping jj workspace root a the canonical version of the command and adding jj root as a more discoverable alias. Feel free to tell me that I'm too stubborn and we should just drop jj workspace root.

@PhilipMetzger
Copy link
Collaborator

Im still in favor of having jj root, while keeping jj workspace root in some way around.

I don't really like the --git flag, though, since that adds Git-knowledge to another command at the same time as we're trying to remove it from jj init (#2747).

I'm fine with having a jj git root as long as we keep the jj $backend <command> pattern consistent, although jj root --git has an easier flow.

Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, but let's wait for consensus on how to continue, I think I've made all my points.

cli/src/commands/workspace.rs Outdated Show resolved Hide resolved
@ilyagr
Copy link
Collaborator

ilyagr commented Jan 16, 2024

I'm OK with any of the options. I don't feel very decisive at the moment.

I wouldn't mind for jj root to be an alias for jj workspace root and jj root --git be an alias for jj git root. I feel like it's slightly weird, but good enough.

One more tangential thought about jj root --repo: the main usecase in my mind is to find the local config file. So, jj config edit --repo (which may or may not already exist) might be a decent substitute. Similarly, perhaps we should have jj ignore edit --repo for the local gitignore.

@v-gb
Copy link
Collaborator Author

v-gb commented Jan 25, 2024

Sorry, I was initially waiting to see if the discussion was going to settle, and context-switched.

The intersection of what everyone is saying seems to be:

  • add jj root, with the same behavior as jj workspace root, and keep referring to jj workspace root in the git-comparison
  • in the future, more flags like --git or --repo may or may not be added to jj root

Although for rare options (how often do people need jj root --git?), a generic interface similar to jj config {list,get} could be preferable

I'd rather not put the git repo in there, as it isn't configurable and just straight up confusing. This also was my previous idea when we talked about something like it.

It doesn't really matter now, but just to clarify, I didn't mean to put things literally in jj config. Just that you could have a place when you stick rarer things. Say, a subcommand info (or whatever else), which provides jj info --all, jj info git-root, jj info more-stuff, etc.

@PhilipMetzger
Copy link
Collaborator

It doesn't really matter now, but just to clarify, I didn't mean to put things literally in jj config. Just that you could have a place when you stick rarer things. Say, a subcommand info (or whatever else), which provides jj info --all, jj info git-root, jj info more-stuff, etc.

Yeah, that definitely makes sense. I'm really sorry for missunderstanding your intent and only focusing on the jj config part.

@v-gb v-gb force-pushed the push-ozxsyrvuklqw branch from a3fefcf to ceca905 Compare January 25, 2024 19:44
Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks for your patience :)

This is a convenient command, for scripting things like `cd $(jj root)
&& do something`, and it seems better to allow people to find it
before they learn about workspaces.
@v-gb v-gb force-pushed the push-ozxsyrvuklqw branch from ceca905 to 2535b67 Compare January 25, 2024 21:23
@v-gb v-gb enabled auto-merge (rebase) January 25, 2024 21:25
@v-gb v-gb merged commit 3b7678d into jj-vcs:main Jan 25, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants